Codecov Report

❌ Patch coverage is 28.57143% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.78%. Comparing base (80fe0c5) to head (1da3818).

@@           Coverage Diff           @@
##             main     #265   +/-   ##
=======================================
  Coverage   78.77%   78.78%           
=======================================
  Files         186      186           
  Lines       18685    18687    +2     
=======================================
+ Hits        14720    14723    +3     
+ Misses       2742     2741    -1     
  Partials     1223     1223           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Greptile Summary

This PR fixes a significant pipeline-efficiency bug: when runner.Run returned a partial error (e.g. one session's LLM call timed out), both the CLI and MCP handlers returned early before calling MarkConsolidated, leaving all successfully-processed events unmarked and causing them to be reprocessed on every subsequent invocation.

The fix reorders the MarkConsolidated call to execute before the error check in both callers, so progress from successful sessions is always preserved. As a side effect, MarkConsolidated failures are demoted from hard errors to logged warnings in both code paths (matching the now-explicit rationale that a failed mark is recoverable — the worst case is a re-run on those events).

Key changes:

• cmd/floop/cmd_consolidate.go and internal/mcp/handler_consolidate.go: MarkConsolidated now runs unconditionally (guarded by !dryRun, result != nil, and len > 0) before runErr is inspected. A result != nil guard is added, though runner.Run always returns a non-nil *RunResult (initialized as aggregated := &RunResult{} at runner.go:78), so this is defensive rather than strictly required.
• internal/consolidation/runner_test.go: Two new tests added — a unit test confirming result.SourceEventIDs is populated from the successful session on partial failure, and an integration test using a real in-memory SQLite store to verify the end-to-end mark behavior.

Confidence Score: 5/5

Safe to merge; the logic change is minimal, well-reasoned, and the prior logger-inconsistency concern was already addressed.

All remaining findings are P2 style/test-design observations. The core reordering is correct: runner.Run always returns a non-nil result with SourceEventIDs only from fully-processed sessions, so calling MarkConsolidated before the error check is sound. No production behavior regressions identified.

runner_test.go — integration test calls MarkConsolidated under DryRun:true, which is a cosmetic mismatch with the !dryRun guard used by actual callers.

Sequence Diagram

sequenceDiagram
    participant Caller as CLI / MCP Handler
    participant Runner as consolidation.Runner
    participant ES as EventStore

    Caller->>Runner: Run(ctx, evts, graphStore, opts)
    loop per session
        Runner->>Runner: runSession(sess-ok) → SourceEventIDs=[e1,e2]
        Runner->>Runner: runSession(sess-fail) → error
        Runner-->>Caller: result{SourceEventIDs:[e1,e2]}, runErr
    end

    Note over Caller,ES: Fixed order (this PR)
    alt !dryRun && result != nil && len(SourceEventIDs) > 0
        Caller->>ES: MarkConsolidated([e1, e2])
        ES-->>Caller: ok (or warn on failure)
    end
    alt runErr != nil
        Caller-->>Caller: return error (e3 remains unconsolidated)
    end

_{Reviews (3): Last reviewed commit: "test: add coverage for mark-consolidated..." | Re-trigger Greptile}